Add initial support and doc for ad2s1210 #11

apelete · 2023-09-20T10:37:56Z

This adds initial support and doc for AD2S1210 Resolver-to-Digital Converter.

Matlab implementation currently depends on AD2S1210 Linux driver work-in-progress implementation (see AD2S1210 updates #2261).

Consequently, this is an initial support proposal to build upon based on AD2S1210 Linux driver implementation.

+adi/+AD2S1210/Rx.m

tfcollins

All the get* should be get.* methods. For the scale properties, are these writable? How are they used?

apelete · 2023-09-21T15:24:05Z

All the get* should be get.* methods. For the scale properties, are these writable? How are they used?

All renamed as asked.
Scale properties are read-only, these values are a multiplier to be applied to the corresponding "raw" property value to obtain the measured value in specified units.

+adi/+AD2S1210/Rx.m

tfcollins

Need some updates related to the get methods

apelete · 2023-09-26T08:21:16Z

Need some updates related to the get methods

Updated get methods for Dependent properties.

ribdp · 2023-10-04T05:19:04Z

+adi/+AD2S1210/Rx.m

+        ExcitationFrequency = 0;
+    end
+
+    properties (Hidden)


This property is not being used - can be removed.

Attribute was removed from the driver indeed ; removing it here too to stay in sync.

Ah, I wasn't aware ExcitationFrequency had been removed from the driver. I was actually referring to the FramesCount property.

It looks like the attribute is still there in the driver? https://github.com/analogdevicesinc/linux/blob/71da83b0b094a92ed4c2b8c21503638b3557ac56/drivers/staging/iio/resolver/ad2s1210.c#L609

Ah, I wasn't aware ExcitationFrequency had been removed from the driver. I was actually referring to the FramesCount property.

SamplesPerFrame property seems to be mandatory; here is the error message I get whent trying to remove it :

Abstract classes cannot be instantiated. Class 'adi.AD2S1210.Rx' inherits abstract methods or properties but does not implement them. See the list of methods and properties that 'adi.AD2S1210.Rx' must implement if you do not intend the class to be abstract. Abstract properties for class adi.AD2S1210.Rx: SamplesPerFrame % defined in matlabshared.libiio.base

It looks like the attribute is still there in the driver? https://github.com/analogdevicesinc/linux/blob/71da83b0b094a92ed4c2b8c21503638b3557ac56/drivers/staging/iio/resolver/ad2s1210.c#L609

There's actually an upstream proposal to convert it to a channel attribute (thus warranting the need to remove it from the Matlab code for now):
https://lore.kernel.org/all/[email protected]/

CLAassistant · 2023-10-05T05:13:46Z

All committers have signed the CLA.

apelete · 2023-10-09T13:13:21Z

@ribdp @tfcollins AFAIK requested changes were implemented so far : any update on this ?

ribdp · 2023-10-10T08:48:36Z

@ribdp @tfcollins AFAIK requested changes were implemented so far : any update on this ?

Similar comment as here: #12 (comment) . The idea is that running that script would update sysobjs.json, which in turn will be used to generate documentation automatically, for the AD4020 and the AD2S1210

apelete · 2023-10-10T09:18:13Z

Similar comment as here: #12 (comment) . The idea is that running that script would update sysobjs.json, which in turn will be used to generate documentation automatically, for the AD4020 and the AD2S1210

Thanks for the quick reply : latest commit updates sysobjs.json as per your instructions.

ribdp · 2023-10-10T10:18:54Z

Confirmed documentation gets generated properly: https://ribdp.github.io/PrecisionToolboxForked/master/sysobjects/adi.AD2S1210.Rx/

Looks good to me.

apelete · 2023-10-11T10:55:15Z

@ribdp @tfcollins I rebased and signed all commits with GPG to get them verified, can you please merge this pull request into main branch ?
I am not able to do so through GitHub webui, getting the following error when clicking on "Rebase and merge" button :

Merge attempt failed

Base branch requires signed commits. Rebase merges cannot be automatically signed by GitHub

apelete · 2023-10-12T14:51:29Z

@ribdp @tfcollins There may be a merge issue when signing the commits with GPG according to documentation (https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification#signature-verification-for-rebase-and-merge) :

When using the Rebase and Merge option on a pull request, it's important to note that the commits in the head branch are added to the base branch without commit signature verification. When you use this option, GitHub creates a modified commit, using the data and content of the original commit. This means that GitHub didn't truly create this commit, and can't therefore sign it as a generic system user. GitHub doesn't have access to the committer's private signing keys, so it can't sign the commit on the user's behalf.

A workaround for this is to rebase and merge locally, and then push the changes to the pull request's base branch.

Do you want me to squash and merge all commits before you push the resulting single commit into the main branch to work around this issue ?

ribdp · 2023-10-13T12:48:13Z

Hi @apelete I do see another issue with the merge check, although upon looking into it it's not related to this PR's changes. So if you could just resolve the merge conflicts, I'd merge and close both PRs. Squashing might not be necessary now, but if it is I'll do that as well.

This adds initial support for AD2S1210 Resolver-to-Digital Converter. Signed-off-by: Apelete Seketeli <[email protected]>

Signed-off-by: Apelete Seketeli <[email protected]>

apelete · 2023-10-13T14:00:45Z

Hi @apelete I do see another issue with the merge check, although upon looking into it it's not related to this PR's changes. So if you could just resolve the merge conflicts, I'd merge and close both PRs. Squashing might not be necessary now, but if it is I'll do that as well.

@ribdp I rebased on main branch again and solved the conflicts, should be good for merging now.

apelete requested a review from ribdp September 20, 2023 10:38

apelete mentioned this pull request Sep 20, 2023

Add support for AD2S1210 #6

Closed

3 tasks

apelete force-pushed the aseketeli/ad2s1210-support branch 2 times, most recently from 71eab14 to e3f00b7 Compare September 20, 2023 11:07

ribdp requested a review from tfcollins September 20, 2023 11:41

tfcollins reviewed Sep 20, 2023

View reviewed changes